-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
System param config #19208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
System param config #19208
Conversation
$( | ||
// Pretend to add each param to the system alone, see if it conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This double-init hackery was added by @cart #2765 (comment) but it shouldn't be needed.
Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.
Note that doing it like this means the component_access_set.extend
below will add a full copy of SystemMeta
each time. There's no way to easily de-duplicate FilteredAccess
, so this will wind up duplicating any access from earlier parameters in the FilteredAccessSet
. If you have multiple ParamSets
s, it will even grow exponentially!
We do make only a single call like this when using ParamSetBuilder
, though, because that consumes the builder and can't be called multiple times.
bevy/crates/bevy_ecs/src/system/builder.rs
Lines 445 to 446 in 01d2b85
// That means that any `filtered_accesses` in the `component_access_set` will get copied to every `$meta` | |
// and will appear multiple times in the final `SystemMeta`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a rather unfortunate situation. Should have been addressed a long time ago. Sounds like we will have to eventually redesign the interface of SystemParam
to fully fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a rather unfortunate situation. Should have been addressed a long time ago. Sounds like we will have to eventually redesign the interface of
SystemParam
to fully fix this.
Yeah, I think the cleanest solution would be for init
to return an access. Tuples and ParamSet
would merge the child accesses, but tuples would need to check for conflicts first so that they don't hand out conflicting accesses. That has the nice property that the conflict checks are only written in one place!
The problem is that passing a &mut
and doing in-place updates is a lot more efficient, so I think it would be a loss overall.
We might be able to make it work if we first split out state initialization from access calculations, and then split out access calculations into a conflict check followed by an update. So there'd be init_state
that does nothing with access, check_conflicts
that panics if there is a conflict but doesn't update anything, append_access
that adds access to a list without checking for conflicts, and check_conflicts_and_append_access
that has a default impl calling the other two in order.
ParamSet
would delegate check_conflicts
and append_access
to the child parameters, but because check_conflicts
is called before append_access
, they wouldn't check conflicts with each other. Tuples would have append_access
create a temporary access list and call check_conflicts_and_append_access
on each child, then merge the temporary list in at the end. For performance, it would then override check_conflicts_and_append_access
to call check_conflicts_and_append_access
on the child parameters directly. That should compile down to the same thing as today for tuples, would remove the double-init for ParamSet
, and wouldn't require duplicating any code.
But that would be a pretty big refactoring, and I don't think anyone has actually had performance issues with ParamSet::init_access
, so it might not be worth it. (Although I am planning a PR to split out state initialization and access calculation so that we can share the access calculations with system param builders.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have some conceptual overlap with SystemParamBuilder
. For example, your "usage" example can be done today with
let mut world = World::new();
let mut schedule = Schedule::default();
schedule.add_systems(
(LocalBuilder(123_usize), LocalBuilder(456_usize))
.build_state(&mut world)
.build_system(test_system),
);
schedule.run(&mut world);
fn test_system(local: Local<usize>, local2: Local<usize>) {
assert_eq!(*local, 123);
assert_eq!(*local2, 456);
}
What can you do with this that you can't do already with builders? Is there some way we can combine the two concepts so that there is only one way to do this?
$( | ||
// Pretend to add each param to the system alone, see if it conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of double-init, we should be able to init the ParamSet on top of the original SystemMeta, then merge all of them together.
Note that doing it like this means the component_access_set.extend
below will add a full copy of SystemMeta
each time. There's no way to easily de-duplicate FilteredAccess
, so this will wind up duplicating any access from earlier parameters in the FilteredAccessSet
. If you have multiple ParamSets
s, it will even grow exponentially!
We do make only a single call like this when using ParamSetBuilder
, though, because that consumes the builder and can't be called multiple times.
bevy/crates/bevy_ecs/src/system/builder.rs
Lines 445 to 446 in 01d2b85
// That means that any `filtered_accesses` in the `component_access_set` will get copied to every `$meta` | |
// and will appear multiple times in the final `SystemMeta`. |
@chescock This PR addresses an entirely different issue. It gives you the ability to configure a system dynamically, after the system was already built. I'm using For example, in a ScheduleBuildPass #11094 , you're given some The system builder will not help in this case because
One additional benefit of this PR is that, because this PR separates the "default" system state and "initialize" system state, after this PR is merged, we can get rid of the awkward So your example is going to look like let mut schedule = Schedule::default();
schedule.add_systems(
(LocalBuilder(123_usize), LocalBuilder(456_usize))
.build_state() // << No world needed yet!
.build_system(test_system),
);
let mut world = World::new(); // << World can be created later!
schedule.run(&mut world);
fn test_system(local: Local<usize>, local2: Local<usize>) {
assert_eq!(*local, 123);
assert_eq!(*local2, 456);
} |
Can you describe the Updating the local variables of a system that you didn't create sounds like it would break encapsulation! What if the system was relying on the values for soundness? (For contrast, builders can only configure systems if they are exposed as |
@chescock In my particular case, I have a This is very important for the Vulkan-based render backend that I'm working on. Each frame can be broken down into multiple submissions and each submission shares the same set of resources. In order to achieve optimal parallelism for command buffer recording, the scheduler needs to be able to look at the entire render graph and insert submission at the optimal places. This is what the
Because the configuration came from the system schedule itself. Which doesn't exist until all the systems were added.
Because systems in each group needs to be executed in parallel. You can obviously create a resources that contains an array of SubmissionInfo, alongside a map from system to array index, but then each system will have to contend on this resource and nothing can be executed in parallel. You'll also have trouble identifying the systems. By allowing an external actor (in this case the ScheduleBuildPass) to modify system states, we can precisely specify their component access and maximize parallelism.
It won't break encapsulation, because these configurations must be done through config tokens that the systems define. For example, in the case of And if you're saying that, if I have a system that behaves well if In this particular case, the system can prevent such modifications by making it |
Triage: failing tests |
Thanks, that's helpful context!
Right, a |
Any function that interacts with the outside world needs to be able to accept any arguments. A Adding a closure as a system is basically equivalent to registering a callback. Even though the callback function itself is private, the moment you register it, it is already interacting with the outside world, and the input arguments are no longer in your control - because you're not the one calling it. The |
Right, my point is that this is changing the contract of how Bevy will call the function. Today, once a system is built, Bevy promises that a |
To reduce controversial-ness, we could introduce a separate |
Rather than adding the concept of "configuration tokens", could we instead just let users operate directly on the system state? One of my reactivity experiments involved using this pattern: let mut system = IntoSystem::into_system(|local: Local<usize>| {});
system.initialize(world);
let state = system.get_state_mut().unwrap();
*state.0.get() = 10; // get() is required because Local's state is a SyncCell<usize> The only missing API is From there, the big question is "how do expose writing this state to the user". It seems like we could have I think I prefer that over adding new concepts. |
@cart The problem is that Obviously we can define fn get_state_mut(&mut self) -> &mut dyn Any But how does the caller know what to cast it into? And even if the caller does know, when the user adds a new param to this system, the downcast could fail. There's also the problem of encapsulation. Systems may not want to expose all of its internals to the outside world. When the user wants to modify system states, systems and system params may want to ensure that it's done in a well-defined way. My solution is to reverse this and have the caller pass a fn configurate(&mut self, token: &mut dyn Any); The system distributes the token to each of its params. Params who know what it is will successfully downcast and react to it. Params who don't know what it is will do nothing. If multiple params know what it is, (for example if you have multiple I realize that using a big word (like "configuration token") may be a little scary, but really it's nothing other than a |
@ItsDoot This PR is really about the I'm happy to remove the |
Ah yeah. The usage example in the description is done on arguments passed into But your actual motivating scenario (setting this in a ScheduleBuildPass) is post-type-erasure and I agree that isn't covered.
System internals (in their current form) were not designed to be a "public interface" that users interact with at runtime (or a framework for deciding what is public or private). I understand that this is what you're trying to enable here, but its worth carefully considering the implications of doing that. I find the "query needs to unwrap its state on every access" and I understand that this puts your use case in a pickle. But you've kind of kicked a "system lifecycle" hornets nest here. And given how niche this use case is, I'd prefer to approach it carefully and with a vision for the future. I sadly don't have a lot of time to devote to this. If you want to move this forward, can you give some thought to what this might look like, operating under the following assumptions?
Also (as a stretch goal) consider taking into account the fact that Systems might be entities/components going forward. Could we take advantage of that somehow (ex: store configurable state as a component). |
An old draft of mine that reworked the system lifecycle: |
d80a409
to
dc839e5
Compare
I mostly agree with your sentiment here.
Indeed. Each system determines how it wants to interact with the outside world using a select number of public interfaces (Config Tokens). So this is entirely opt-in. When designating the config tokens, receiving systems should modify its state carefully, in a safe and well-defined way. This PR does not propose arbitrary modification of system states.
I also agree. There are actually two usage scenarios:
So I'm cutting down the scope of this PR to only support post-init configuration. No lifetime changes would be necessary if we do it this way. When systems are always initialized by default, there will be no "pre-init configuration" scenario and so it'll naturally be a smooth transition to that end goal. |
Objective
Frequently it is desirable to modify the state of a system after the system was already created. For example, in a
ScheduleBuildPass
(#11094), we're given a list ofBoxedSystem
s and we have no idea what are those systems. There needs to be a way for us to dynamically introspect and configure those systems and their params. We can keep adding methods to theSystem
trait but it's already getting huge. So let's add a general-purposeconfigurate
method to theSystem
trait to cover niche and less frequent use cases.Note that this PR only addresses post-initialization configuration. Pre-init configuration shall be done with other mechanisms like
SystemBuilder
.Solution
Add
System::configurate
andSystemParam::configurate
which takes a&mut dyn Any
as input. We call this&mut dyn Any
a "configuration token". Systems and system params to decide what to do with it.For example, in the case of
Local
, we may be able to add a config tokenLocalConfig
.LocalConfig<T>
changes the value of the first uninitializedLocal<T>
.Another example. In my application I can use a
ScheduleBuildPass
to modify system states such that some systems have theirResMut<MyState>
SystemParam point to an entirely different instance if they're located in a special SystemSet.Testing
All bevy_ecs tests passing.